I tried to rewrite this with early return, but failed -- only way I saw to return early is to duplicate one of the if clauses, which was, imo, worse.

This if-else block seems to be doing a "set inputRoot and nodeProperties based on the value of actionRoot". That sounds like a good candidate for a helper function that would facilitate using less indentation.

Yes, thank you, good idea! I did one better, I think -- I split the input root handling and the node properties, because they have nothing to do with each other. PTAL.

I think the proper way of supporting v2.1 is to follow the same deprecation semantics by adding an OutputPaths field, removing the deprecated ones, and handling the precedence during this conversion here as defined in the API.

I'm concerned about unverified use-cases that might break by nilling OutputDirs and assuming all paths fit into OutputFiles.

WDYT?

Problem is, that deprecation never really worked out. Bazel still uses both fields (I see some recent action trying to move towards using one, but no resolution). I suspect Google's RBE still uses files/dirs. So we need to support both fields as much as possible in this client until version 3.0 actually removes the deprecated fields (and not sure if that'll ever happen...).

Here is what I think we should do:

• when composing an entirely new RE-API proto, we should decide which fields to use based on server capabilities (which we already do AFAICT).
• when parsing an RE-API proto, we need to work correctly with all versions: if output_paths is set, it should supercede output_files/dirs. Which is what I implement here.
• the composing and parsing should be orthogonal -- the tool should support reading protos in one format and outputting them in another (e.g. action was downloaded from a different RE than it is executed on, or the RE migrates to a different API version)

The previous implementation didn't support output_paths at all, which meant this tool was not working with servers supporting 2.1, and here I fix that. AFAICT we can indeed safely put all outputs in output_files, because we shouldn't care what they really are; if there is any code that will be broken by this assumption, we need to find out ASAP and fix it, because we actually can't know the type of the output. But I believe that's extremely unlikely. Basically, the only reason these exist as split fields in Command to begin with is because they were split in the RE-API and we needed to support that. All we do is pass them through to RE. We do no semantic checks on them.

Adding an OutputPaths field in Command is possible, but imo it will only add complexity, as we'll still need to populate the RE proto based on all fields, and maintain the fields in sync. I think just using the output files field is simpler. Of course, if someone tries to run a saved 2.1 command with an output directory on a 2.0 RE, this will not work. without them manually modifying the saved proto, but that's true regardless of what field we use.

Still TODO, btw:

• make the fake server support both RE-API versions (via an option)
• use parameterized tests to test both, to make sure that we support all combinations (can run old commands on new servers, and vice versa).
• improve the tool to be more explicit when the action, command and input root digests actually change from what has been requested / found on disk. The tool currently silently recomputes a lot of digests, which can be misleading and cover up errors such as API version mismatches leading to different digests.

Thanks for the well laid-out comment.

My concern was specifically around losing the information about which paths are directories. As you mentioned, this is only a problem when passing a command from 2.1+ server to a 2.0- one, in which case the information (directories) wasn't or shouldn't be there in the first place.

This if-else block seems to be doing a "set inputRoot and nodeProperties based on the value of actionRoot". That sounds like a good candidate for a helper function that would facilitate using less indentation.

Can you rearrange the conditions such that the terminal one appears first?

Yes, much better now, thank you! PTAL.

Thanks for the well laid-out comment.

My concern was specifically around losing the information about which paths are directories. As you mentioned, this is only a problem when passing a command from 2.1+ server to a 2.0- one, in which case the information (directories) wasn't or shouldn't be there in the first place.